Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gentype: make output DCE-friendly #6508

Merged
merged 2 commits into from
Dec 18, 2023
Merged

Gentype: make output DCE-friendly #6508

merged 2 commits into from
Dec 18, 2023

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Dec 5, 2023

Background

I recently discovered a problem while using gentype.

When using ES6 output, gentype assigns an imported namespace to a variable.

import * as ModuleBS__Es6Import from './Module.bs';
const ModuleBS: any = ModuleBS__Es6Import;

This harms DCE. ESM bundlers optimize module dependencies when they can be statically analyzed. However, if indirection such as alias variables is added, it will be deopted.

For example:

// Dep1.js
export function fn1() {
  console.log('Dep1.fn1');
}
export function fn2() {
  console.log('Dep1.fn2');
}
// Dep2.js
export function fn1() {
  console.log('Dep2.fn1');
}
export function fn2() {
  console.log('Dep2.fn2');
}
// Module.js
import * as Dep1 from './Dep1.js';

import * as Dep2_Import from './Dep2.js';
const Dep2 = Dep2_Import;

Dep1.fn1();
Dep2.fn1();

When exec esbuild src/Module.js --bundle --format=esm --target=node18 --outfile=bundle.js

// bundle.js
var __defProp = Object.defineProperty;
var __export = (target, all) => {
  for (var name in all)
    __defProp(target, name, { get: all[name], enumerable: true });
};

// src/Dep1.js
function fn1() {
  console.log("Dep1.fn1");
}

// src/Dep2.js
var Dep2_exports = {};
__export(Dep2_exports, {
  fn1: () => fn12,
  fn2: () => fn2
});
function fn12() {
  console.log("Dep2.fn1");
}
function fn2() {
  console.log("Dep2.fn2");
}

// src/Module.js
var Dep2 = Dep2_exports;
fn1();
Dep2.fn1();

esbuild doesn't do DCE for Dep2.js module.

This change makes gentype output to be more friendly with bundlers.

  • Q. What is const ModuleBS: any = ModuleBS__Es6Import; anyway?

    • IDK exactly, I guess it was for suppressing type errors from untyped modules. But regardless of this, users get an error in the import statement. if there are no allowJs: true.

  • Q. Why as any is needed for every binding?

    • Because depending on the strictness settings, TypeScript attempts basic type inference in JS as well. And it may fail for string literals, abstract data types, etc. Even if it is safe.

  • Q. Why are new errors reported in make test-gentype after making changes?

    • They are actually existing bugs in the gentype's "runtime mode" features. Could not be found while suppressing type errors in the entire module.

cometkim
cometkim commented Dec 5, 2023
View reviewed changes
jscomp/gentype_tests/typescript-react-example/src/ExportWithRename.gen.tsx Outdated

export type Props = { readonly s: string };

export const make: React.ComponentType<{ readonly s: string }> = ExportWithRenameBS.ExportWithRename;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript report type error on this

Property 'ExportWithRename' does not exist on type 'typeof import(".../ExportWithRename.bs")'.

Which is accurate.

That may be the intended behavior of @gentType.as, but it doesn't actually work. If it's not a bug, I can't even fix it.

@cometkim
Copy link
Member Author

cometkim commented Dec 5, 2023 •
edited
Loading

The test fails due to an unrelated bug mentioned above. Maybe another PR to fix it? or can this be merged first?
Fixed in #6509

@cometkim cometkim mentioned this pull request Dec 5, 2023
Merged
@zth zth requested a review from cristianoc December 5, 2023 20:49
cometkim
cometkim commented Dec 6, 2023
View reviewed changes
jscomp/gentype_tests/typescript-react-example/src/FirstClassModulesInterface.gen.tsx Outdated

export type record = { readonly x: number; readonly y: string };

export type firstClassModule = { readonly x: number };

export const MT_x: number = FirstClassModulesInterfaceBS.MT.x;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript report on this

error TS2339: Property 'MT' does not exist on type 'typeof import("/Users/runner/work/rescript-compiler/rescript-compiler/jscomp/gentype_tests/typescript-react-example/src/FirstClassModulesInterface.bs")'

because MT is actually module type, so it should be handled as a type, not a value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handling first class modules appears to be a partial implementation.

The translator part is a bit new to me, I'm spending some time debugging and tracing the flow. I will make a PR in this week.

@cometkim
Copy link
Member Author

cometkim commented Dec 7, 2023

BTW bugs found here imply that it hasn't had any real users for a long time.

Maybe we'll feel a bit more confident in deprecating gentype advanced features? @cristianoc

@cristianoc
Copy link
Collaborator

That was never fully supported. Perhaps just delete the test?

@cometkim
Copy link
Member Author

cometkim commented Dec 7, 2023

I'm concerned about is that if a user doesn't actually call it at runtime, but generates the type, then sees type errors from TypeScript that the user didn't get before.

Is it too much to worry about?

@cristianoc
Copy link
Collaborator

@cometkim I think the issue is that module types also export values, while they should only export types.
Here's a PR trying to address that: #6516
How does that look?

@cometkim
Copy link
Member Author

cometkim commented Dec 9, 2023

OK that looks better, I thought it was awkward to add a feature to deprecate

@cristianoc
Copy link
Collaborator

OK that looks better, I thought it was awkward to add a feature to deprecate

Are there other outstanding issues after that PR?

@mununki
Copy link
Member

mununki commented Dec 12, 2023

Some real user here. 😃

@cometkim
Copy link
Member Author

Are there other outstanding issues after that PR?

I think nothing left then. But let me rebase this PR first to make sure

@cometkim
Copy link
Member Author

@cristianoc ready to merge

@cristianoc
Copy link
Collaborator

@cristianoc ready to merge

Great.
Any risks or other things to check, perhaps with someone else's project? Just in case new errors might begin to appear after this change.

@cometkim
Copy link
Member Author

@mununki could you try this on your project?

@mununki
Copy link
Member

mununki commented Dec 15, 2023

I confirm that the gentype ouput looks intended as this PR proposes. But I have still same TS build error as below:

error TS2307: Cannot find module './AddressSelect' or its corresponding type declarations.

6 import * as AddressSelectBS from './AddressSelect';

This issue is opened #6521

@cometkim
Copy link
Member Author

It was introduced by #6442 and is not directly related to this change. correct?

@mununki
Copy link
Member

mununki commented Dec 18, 2023

The issue #6521 is related to the gentype configuration.

@cristianoc cristianoc merged commit 7890609 into rescript-lang:master Dec 18, 2023
@cometkim cometkim deleted the gentype-dce branch December 18, 2023 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristianoc cristianoc Awaiting requested review from cristianoc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cometkim @cristianoc @mununki